Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only save feature properties when actually needed #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perliedman
Copy link
Member

We store feature properties in our layers for use when a layer is interactive and/or can change feature styles after they're loaded. However, when this feature isn't enabled, storing the properties is unnecessary and can take up a lot of memory.

This PR makes sure properties are only saved when we actually need them.

@jkuebart
Copy link
Contributor

jkuebart commented Feb 15, 2017

Note that other than through getFeatureId(), properties on layers are also visible to event handlers such as click. Only making them available if getFeatureId() is specified might break some client code.

Maybe the condition options.getFeatureId || options.interactive might be strong enough?

@jkuebart
Copy link
Contributor

Adding some unit tests for this project might be worthwhile, and I'd be willing to write one for my assertion above, but I'm afraid setting up the framework from scratch is too daunting to me…

@perliedman
Copy link
Member Author

You're right, should check interactive as well. It sort of hightlights that the interactive and getFeatureId aren't 100% orthogonal: I have a hard time coming up with a use case where you would set interactive: true but not supply getFeatureId.

@jkuebart
Copy link
Contributor

In my case, clicking a feature triggers an update in a separate window without the need for any feedback in the map itself. I can work out which feature was clicked from its properties, but since I don't have a need for setFeatureStyle() I don't supply getFeatureId.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants